Introduce ApplicationRecord, an Active Record layer supertype #22567

gsamokovarov · 2015-12-12T14:34:16Z

It's pretty common for folks to monkey patch ActiveRecord::Base to
work around an issue or introduce extra functionality. Instead of
shoving even more stuff in ActiveRecord::Base, ApplicationRecord can
hold all those custom work the apps may need.

Now, we don't wanna encourage all of the application models to inherit
from ActiveRecord::Base, but we can encourage all the models that do,
to inherit from ApplicationRecord.

Newly generated applications have app/models/application_record.rb
present by default. The model generators are smart enough to recognize
that newly generated models have to inherit from ApplicationRecord,
but only if it's present.

rails-bot · 2015-12-12T14:34:18Z

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

gsamokovarov · 2015-12-12T14:37:22Z

r? @rafaelfranca

claudiob · 2015-12-12T17:08:40Z

Do we need to test the case in which a user creates a model called ApplicationRecord?

It might not be too far-fetched as an option. Maybe 'ApplicationRecord' can be included in the words that cannot be used as a name for a model. Or a warning could be displayed if such a model exists.

gsamokovarov · 2015-12-12T17:25:55Z

Do we need to test the case in which a user creates a model called ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the NamedBase generators. For example if you do bundle exec rails generate controller Application, you end up with:

class ApplicationController < ApplicationController
end

rafaelfranca · 2015-12-12T22:28:01Z

This is way too defensive to me. I don't think anyone would have a model
called ApplicationRecord

On Sat, Dec 12, 2015, 15:26 Genadi Samokovarov notifications@github.com
wrote:

Do we need to test the case in which a user creates a model called
ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the
NamedBase generators. For example if you do bundle exec rails generate
controller Application, you end up with:

class ApplicationController < ApplicationControllerend

—
Reply to this email directly or view it on GitHub
#22567 (comment).

claudiob · 2015-12-13T02:01:48Z

in that case, no objections 🎀

kaspth · 2015-12-13T13:54:15Z

activerecord/CHANGELOG.md

+    It's pretty common for folks to monkey patch `ActiveRecord::Base` to
+    work around an issue or introduce extra functionality. Instead of
+    shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can
+    hold all those custom work the apps may need.


I think we should rephrase the motivation. What I've understood to be the issue is that engines will force configuration changes onto an application, and that there's no way to keep them separate because both were configuring ActiveRecord::Base. That, in turn, results in monkey patches.

You are right about the convenience of where to include extra app wide record functionality.

cc @rafaelfranca

Yes! We can and should advertise that to plugin authors. I have AR extensions myself, which I can distribute as modules to be mixed into ApplicationRecord instead of ActiveRecord::Base monkey patches.

Yes we need to rephrase the motivation to talk about engines.

My point wasn't whether or not we should advertise to engine authors. It was that the motivation stated here isn't why we want to add this class.

claudiob · 2015-12-15T17:28:59Z

As part of this commit, you might also want to change these lines of the README so people learn not to inherit directly from ActiveRecord::Base anymore:

The library provides a base class that, when subclassed, sets up a mapping between the new class and an existing table in the database.

claudiob · 2015-12-15T17:31:00Z

I am also wondering whether we should change tests/guides/docs to refrain from referencing ActiveRecord::Base whenever possible in favor of ApplicationRecord.

gsamokovarov · 2015-12-15T18:02:21Z

Yo! I tried to touch a bit on the documentation, I'm sure I haven't replaced all the references, but at least the Active Record guides are all covered. Still, the diff is reasonable and isn't blown up.

@kaspth can you check the changelog entry again?

gsamokovarov · 2015-12-15T18:07:47Z

guides/source/plugins.md

-ActiveRecord::Base.include(Yaffle::ActsAsYaffle)
+# test/dummy/app/models/application_record.rb
+
+class ApplicationRecord < ActiveRecord::Base


I tried to sell this, so I can hint the plugin folks. Tell me if you think it's an overkill.

It's pretty common for folks to monkey patch `ActiveRecord::Base` to work around an issue or introduce extra functionality. Instead of shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can hold all those custom work the apps may need. Now, we don't wanna encourage all of the application models to inherit from `ActiveRecord::Base`, but we can encourage all the models that do, to inherit from `ApplicationRecord`. Newly generated applications have `app/models/application_record.rb` present by default. The model generators are smart enough to recognize that newly generated models have to inherit from `ApplicationRecord`, but only if it's present.

gsamokovarov · 2015-12-16T10:00:52Z

activerecord/CHANGELOG.md

@@ -1,3 +1,18 @@
+*   Introduce ApplicationRecord, an Active Record layer super type.
+
+    An `ApplicationRecord` let's engines have models, isolated from the main


Can you folks pass over the motivation again?

Introduce ApplicationRecord, an Active Record layer supertype

rafaelfranca · 2015-12-16T16:38:00Z

❤️ 💚 💙 💛 💜

gsamokovarov · 2015-12-16T16:51:01Z

Thanks for the help, guys! I'll try to follow up with more documentation changes.

jeremy · 2015-12-16T20:28:23Z

activerecord/lib/rails/generators/active_record/model/model_generator.rb

        end

+        def determine_default_parent_class
+          if File.exist?('app/models/application_record.rb')


Is it safe to expect that the app root is the current working directory here, or should it check for the file presence relative to application root?

I'll check if thor has a helper for that

I made the check to be relative to the application root here 342221b

rails-bot assigned chancancode Dec 12, 2015

rails-bot assigned rafaelfranca and unassigned chancancode Dec 12, 2015

gsamokovarov force-pushed the introduce-application-record branch 3 times, most recently from 2ed43e3 to 851deb6 Compare December 12, 2015 16:50

gsamokovarov force-pushed the introduce-application-record branch from 851deb6 to 5059dd6 Compare December 13, 2015 10:33

kaspth reviewed Dec 13, 2015
View reviewed changes

gsamokovarov force-pushed the introduce-application-record branch 2 times, most recently from c533f43 to 84da9c0 Compare December 15, 2015 17:56

gsamokovarov reviewed Dec 15, 2015
View reviewed changes

gsamokovarov force-pushed the introduce-application-record branch 2 times, most recently from ce76f20 to 762e4df Compare December 16, 2015 09:23

gsamokovarov force-pushed the introduce-application-record branch from 762e4df to 2067fff Compare December 16, 2015 09:31

gsamokovarov reviewed Dec 16, 2015
View reviewed changes

rafaelfranca added a commit that referenced this pull request Dec 16, 2015

Merge pull request #22567 from gsamokovarov/introduce-application-record

1d7d806

Introduce ApplicationRecord, an Active Record layer supertype

rafaelfranca merged commit 1d7d806 into rails:master Dec 16, 2015

jeremy reviewed Dec 16, 2015
View reviewed changes

HoyaBoya mentioned this pull request Dec 18, 2015

Make It Easier To Extend AASM With Custom Logic Across Models aasm/aasm#296

Merged

namiwang mentioned this pull request Mar 8, 2016

support for ApplicationRecord in rails 5 state-machines/state_machines-activerecord#39

Closed

simmerer mentioned this pull request Jul 15, 2016

look up ActiveRecord::Base.descendants instead of .subclasses vigetlabs/sprig-reap#27

Open

kenchan0130 mentioned this pull request May 14, 2017

Support rails5 for templates gocardless/statesman#243

Closed

1 task

hmadison mentioned this pull request Jan 29, 2018

Thread read notifications lobsters/lobsters#417

Merged

kennyadsl mentioned this pull request Dec 9, 2019

Explore using ApplicationRecord instead of ActiveRecord::Base as models parent class solidusio/solidus#3450

Closed

diegoviola mentioned this pull request Jan 22, 2020

Rails 6 benwbrum/fromthepage#1567

Merged

Hamms mentioned this pull request Nov 24, 2020

Update all existing models to inherit from ApplicationRecord code-dot-org/code-dot-org#37997

Merged

8 tasks

camallen mentioned this pull request Oct 19, 2022

use ApplicationRecord as model inheritance base zooniverse/panoptes#3979

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ApplicationRecord, an Active Record layer supertype #22567

Introduce ApplicationRecord, an Active Record layer supertype #22567

gsamokovarov commented Dec 12, 2015

rails-bot commented Dec 12, 2015

gsamokovarov commented Dec 12, 2015

claudiob commented Dec 12, 2015

gsamokovarov commented Dec 12, 2015

rafaelfranca commented Dec 12, 2015

claudiob commented Dec 13, 2015

kaspth Dec 13, 2015

gsamokovarov Dec 13, 2015

rafaelfranca Dec 14, 2015

kaspth Dec 14, 2015

claudiob commented Dec 15, 2015

claudiob commented Dec 15, 2015

gsamokovarov commented Dec 15, 2015

gsamokovarov Dec 15, 2015

gsamokovarov Dec 16, 2015

rafaelfranca commented Dec 16, 2015

gsamokovarov commented Dec 16, 2015

jeremy Dec 16, 2015

rafaelfranca Dec 16, 2015

rafaelfranca Dec 16, 2015

		@@ -1,3 +1,18 @@
		* Introduce ApplicationRecord, an Active Record layer super type.

		An `ApplicationRecord` let's engines have models, isolated from the main

Introduce ApplicationRecord, an Active Record layer supertype #22567

Introduce ApplicationRecord, an Active Record layer supertype #22567

Conversation

gsamokovarov commented Dec 12, 2015

rails-bot commented Dec 12, 2015

gsamokovarov commented Dec 12, 2015

claudiob commented Dec 12, 2015

gsamokovarov commented Dec 12, 2015

rafaelfranca commented Dec 12, 2015

claudiob commented Dec 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiob commented Dec 15, 2015

claudiob commented Dec 15, 2015

gsamokovarov commented Dec 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Dec 16, 2015

gsamokovarov commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment